Veto plugins #465
Veto plugins #465
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
pep8
straxen/plugins/veto_vetos.py|97 col 5| WPS331 Found variables that are only used for return: res
straxen/plugins/veto_vetos.py|101 col 1| WPS211 Found too many arguments: 7 > 5
straxen/plugins/veto_vetos.py|112 col 35| W503 line break before binary operator
straxen/plugins/veto_vetos.py|113 col 35| W503 line break before binary operator
straxen/plugins/veto_vetos.py|122 col 1| E302 expected 2 blank lines, found 1
straxen/plugins/veto_vetos.py|140 col 34| WPS303 Found underscored number: 1_000
straxen/plugins/veto_vetos.py|144 col 8| N801 class name 'muVETOveto' should use CapWords convention
straxen/plugins/veto_vetos.py|145 col 1| D204 1 blank line required after class docstring
straxen/plugins/veto_vetos.py|160 col 1| D102 Missing docstring in public method
straxen/plugins/veto_vetos.py|163 col 22| W503 line break before binary operator
straxen/plugins/veto_vetos.py|165 col 1| D102 Missing docstring in public method
straxen/plugins/veto_vetos.py|166 col 16| WPS613 Found incorrect super() call context: incorrect name access
straxen/plugins/veto_vetos.py|166 col 53| W292 no newline at end of file
tests/test_nveto_recorder.py|6 col 1| I003 isort expected 1 blank line in imports, found 0
tests/test_nveto_recorder.py|8 col 1| D101 Missing docstring in public class
tests/test_nveto_recorder.py|10 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|13 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|13 col 16| WPS507 Found useless len() compare
tests/test_nveto_recorder.py|15 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|32 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|33 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|34 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|35 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|36 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|39 col 1| D101 Missing docstring in public class
tests/test_nveto_recorder.py|41 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|55 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|58 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|58 col 16| WPS507 Found useless len() compare
tests/test_nveto_recorder.py|60 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|61 col 101| E501 line too long (105 > 100 characters)
tests/test_nveto_recorder.py|62 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|63 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|64 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|64 col 101| E501 line too long (103 > 100 characters)
tests/test_nveto_recorder.py|66 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|66 col 5| WPS218 Found too many assert statements: 6 > 5
tests/test_nveto_recorder.py|67 col 101| E501 line too long (105 > 100 characters)
tests/test_nveto_recorder.py|68 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|69 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|70 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|72 col 101| E501 line too long (105 > 100 characters)
tests/test_nveto_recorder.py|73 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|74 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|75 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|1 col 1| F401 'strax' imported but unused
tests/test_veto_vetos.py|1 col 1| I003 isort expected 1 blank line in imports, found 0
tests/test_veto_vetos.py|2 col 1| I003 isort expected 1 blank line in imports, found 0
tests/test_veto_vetos.py|7 col 1| E302 expected 2 blank lines, found 1
tests/test_veto_vetos.py|7 col 1| D101 Missing docstring in public class
tests/test_veto_vetos.py|8 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|13 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|16 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|16 col 16| WPS507 Found useless len() compare
tests/test_veto_vetos.py|18 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|40 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|41 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|42 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|43 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|44 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|46 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|54 col 62| E241 multiple spaces after ','
tests/test_veto_vetos.py|59 col 5| E303 too many blank lines (2)
tests/test_veto_vetos.py|60 col 5| WPS602 Found using @staticmethod
tests/test_veto_vetos.py|71 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|71 col 16| WPS507 Found useless len() compare
tests/test_veto_vetos.py|71 col 101| E501 line too long (116 > 100 characters)
tests/test_veto_vetos.py|78 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|78 col 101| E501 line too long (105 > 100 characters)
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
pep8
tests/test_nveto_recorder.py|63 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|64 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|64 col 101| E501 line too long (103 > 100 characters)
tests/test_nveto_recorder.py|66 col 1| D102 Missing docstring in public method
tests/test_nveto_recorder.py|66 col 5| WPS218 Found too many assert statements: 6 > 5
tests/test_nveto_recorder.py|67 col 101| E501 line too long (105 > 100 characters)
tests/test_nveto_recorder.py|68 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|69 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|70 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|72 col 101| E501 line too long (105 > 100 characters)
tests/test_nveto_recorder.py|73 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|74 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_nveto_recorder.py|75 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|1 col 1| F401 'strax' imported but unused
tests/test_veto_vetos.py|7 col 1| E302 expected 2 blank lines, found 1
tests/test_veto_vetos.py|7 col 1| D101 Missing docstring in public class
tests/test_veto_vetos.py|8 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|13 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|16 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|16 col 16| WPS507 Found useless len() compare
tests/test_veto_vetos.py|18 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|40 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|41 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|42 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|43 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|44 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|46 col 1| D102 Missing docstring in public method
tests/test_veto_vetos.py|54 col 62| E241 multiple spaces after ','
tests/test_veto_vetos.py|59 col 5| E303 too many blank lines (2)
tests/test_veto_vetos.py|71 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|71 col 16| WPS507 Found useless len() compare
tests/test_veto_vetos.py|71 col 101| E501 line too long (116 > 100 characters)
tests/test_veto_vetos.py|78 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|78 col 101| E501 line too long (105 > 100 characters)
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
pep8
tests/test_veto_vetos.py|71 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|71 col 16| WPS507 Found useless len() compare
tests/test_veto_vetos.py|71 col 101| E501 line too long (116 > 100 characters)
tests/test_veto_vetos.py|78 col 1| S101 Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
tests/test_veto_vetos.py|78 col 101| E501 line too long (105 > 100 characters)
straxen/plugins/veto_vetos.py
Outdated
|
|
||
| MV_PREAMBLE = 'Muno-Veto Plugin: Same as the corresponding nVETO-PLugin.\n' | ||
|
|
||
| @export |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
E302 expected 2 blank lines, found 1
There was a problem hiding this comment.
@WenzDaniel , I actually like this convention for readability, you disagree?
There was a problem hiding this comment.
Thanks @WenzDaniel I think overall it looks good.
One general question, you decided against merging this plugin with the plugins from https://github.com/XENONnT/straxen/pull/487/files since we at the moment do not know the correct settings or is there a more fundamental reasoning behind this?
Just mostly minor comments below:
| @@ -105,29 +105,37 @@ def compute(self, raw_records_nv, start, end): | |||
| min_amplitude=self.config['hit_min_amplitude_nv']) | |||
| del temp_records | |||
|
|
|||
There was a problem hiding this comment.
medium: I think you need to also increment the version. The code is not entirely the same for the _merge_intervals
There was a problem hiding this comment.
The resulting raw_records_coin_nv should be identical, but we can also increase the version number for bookkeeping. It will just mean a lot of reprocessing.
There was a problem hiding this comment.
if you are sure that the results are the same, I stand corrected and no version bump would be needed
There was a problem hiding this comment.
I checked with a 6 min run (42 million raw_record fragments and 1.6 million raw_records_coin_nv). Shape and all entries are identical.
straxen/plugins/veto_vetos.py
Outdated
|
|
||
| MV_PREAMBLE = 'Muno-Veto Plugin: Same as the corresponding nVETO-PLugin.\n' | ||
|
|
||
| @export |
There was a problem hiding this comment.
@WenzDaniel , I actually like this convention for readability, you disagree?
Co-authored-by: Joran Angevaare <jorana@nikhef.nl>
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
pep8
tests/test_nveto_recorder.py|102 col 44| WPS204 Found overused expression: self.intervals['time']; used 6 > 4
tests/test_veto_veto_regions.py|6 col 1| D101 Missing docstring in public class
tests/test_veto_veto_regions.py|7 col 1| D102 Missing docstring in public method
tests/test_veto_veto_regions.py|19 col 1| D102 Missing docstring in public method
tests/test_veto_veto_regions.py|22 col 16| WPS507 Found useless len() compare
tests/test_veto_veto_regions.py|24 col 1| D102 Missing docstring in public method
tests/test_veto_veto_regions.py|31 col 101| E501 line too long (102 > 100 characters)
tests/test_veto_veto_regions.py|32 col 101| E501 line too long (104 > 100 characters)
tests/test_veto_veto_regions.py|35 col 9| WPS221 Found line with high Jones Complexity: 16 > 14
tests/test_veto_veto_regions.py|35 col 69| E226 missing whitespace around arithmetic operator
tests/test_veto_veto_regions.py|37 col 9| WPS221 Found line with high Jones Complexity: 16 > 14
tests/test_veto_veto_regions.py|40 col 9| WPS221 Found line with high Jones Complexity: 17 > 14
tests/test_veto_veto_regions.py|42 col 9| WPS221 Found line with high Jones Complexity: 17 > 14
tests/test_veto_veto_regions.py|45 col 1| D102 Missing docstring in public method
tests/test_veto_veto_regions.py|64 col 16| WPS507 Found useless len() compare
tests/test_veto_veto_regions.py|64 col 101| E501 line too long (116 > 100 characters)
tests/test_veto_veto_regions.py|71 col 101| E501 line too long (105 > 100 characters)
| 'min_veto_channel_nv', default=0, type=int, track=True, | ||
| help='Minimal number PMT channel contributing to the n/mveto_event.'), | ||
| strax.Option( | ||
| 'veto_left_extension_nv', default=500_000, type=int, track=True, |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
WPS303 Found underscored number: 500_000
|
|
||
| time_is_correct = intervals[-1]['time'] == self.intervals['time'][-1] | ||
| assert time_is_correct, 'Second interval has the wrong time!' | ||
| time_is_correct = intervals[-1]['endtime'] == self.intervals['endtime'][-1] |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
WPS221 Found line with high Jones Complexity: 16 > 14
tests/test_nveto_recorder.py
Outdated
| truth_time = np.array([self.intervals['time'][0], | ||
| self.intervals['time'][-1]]) | ||
| truth_endtime = np.array([self.intervals['time'][-2], | ||
| self.intervals['time'][-1]]) + resolving_time, |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
C818 trailing comma on bare tuple prohibited
| coincidence=3, | ||
| pre_trigger=0, | ||
| n_concidences_truth=1, | ||
| times_truth=self.intervals['time'][0], |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
WPS204 Found overused expression: self.intervals['time']; used 6 > 4
| import unittest | ||
|
|
||
|
|
||
| class TestCreateVetoIntervals(unittest.TestCase): |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
D101 Missing docstring in public class
|
|
||
|
|
||
| class TestCreateVetoIntervals(unittest.TestCase): | ||
| def setUp(self): |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method
|
|
||
| time_is_correct = vetos[1]['time'] == self.events['time'][-1] - left_extension | ||
| assert time_is_correct, 'Second veto event has the wrong time!' | ||
| time_is_correct = vetos[1]['endtime'] == self.events['endtime'][-1] + right_extension |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
WPS221 Found line with high Jones Complexity: 17 > 14
| time_is_correct = vetos[1]['endtime'] == self.events['endtime'][-1] + right_extension | ||
| assert time_is_correct, 'Second veto event has the wrong endtime!' | ||
|
|
||
| def test_thresholds(self): |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method
| left_extension=0, | ||
| right_extension=0) | ||
| print(events[field], thresholds, vetos) | ||
| assert len(vetos) == 0, f'Vetos for {threshold_type} threshold should be empty since it is below threshold!' |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
WPS507 Found useless len() compare
| left_extension=0, | ||
| right_extension=0) | ||
| print(events[field], thresholds, vetos) | ||
| assert len(vetos) == 0, f'Vetos for {threshold_type} threshold should be empty since it is below threshold!' |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
E501 line too long (116 > 100 characters)
| **thresholds, | ||
| left_extension=0, | ||
| right_extension=0) | ||
| assert len(vetos) == 1, f'{threshold_type} threshold did not work, have a wrong number of vetos!' |
There was a problem hiding this comment.
[pep8] reported by reviewdog 🐶
E501 line too long (105 > 100 characters)
Hm mainly two reasons. Back then it was not fully clear which way to go either tag all S1 Peaks or just the one classified as main/alt S1 in event basics. The second reason, I did not want extend this PR with another feature as it is already quite long. |
There was a problem hiding this comment.
One general question, you decided against merging this plugin with the plugins from https://github.com/XENONnT/straxen/pull/487/files since we at the moment do not know the correct settings or is there a more fundamental reasoning behind this?
Hm mainly two reasons. Back then it was not fully clear which way to go either tag all S1 Peaks or just the one classified as main/alt S1 in event basics. The second reason, I did not want extend this PR with another feature as it is already quite long.
I mean, if you considered making it a single plugin instead of multiple?
Other than that, I think you looked at all the comments right without any controversies?
We may want to replace and/or extend the way we decide when to veto a certain events. Hence I thought it would be simpler to distinguish between the vetoing and tagging part.
No, no objects here. I implemented them. |
yes, for the development this is certainly true. I don't know how big the penalty (if there exists one) is for having to communicate between plugins is. |
What is the problem / what does the code in this PR do
In this PR we are adding a veto plugin for the neutron and muon-veto. The veto plugins define the regions in which S1 peaks would be tagged by the muon- or neutron-veto. The corresponding event plugin for the TPC will be shipped in an additional PR. The corresponding tests had been added too.
To avoid any duplicated code, I had to refactored the function _merge_intervals in the nveto recorder. Due to this modification I also had to slightly adjust the nveto_recorder plugin as well as the function find_coincidence. Since neither merge_intervals nor find_coincidence was tested by unit tests yet. I also added those.